Repository navigation
fix(workflow): carry the run id into the history row a graph edit archives - #1613
Conversation
…hinkFlowLab#1611) saveGraph ends a run when the edit drops a node it was running, and archives that attempt as a history row, but the row carried no id. The Runs tab skips a row without one, so the stopped attempt could never be marked as superseded by the retry that replaced it (ThinkFlowLab#1604). The demo-store migration writes its row the same way; it now takes the row builder too, keeps the stored run id when the record has one, and writes no id when it does not — the map key there is the lane, never a run, so it is not borrowed as one. Docs-not-needed: docs/subsystems/workflow.md already describes the lane history and the Runs tab; this only puts the id the client already reads into the row. Refs ThinkFlowLab#1611
|
@Birfy review request — PR #1613 at
As of my read: 3 files +181/−4, base |
|
Verdict: PASS at c3861bf Ran at this tip: Scope: Red first: with only The fix itself: Checks at this tip: Non-blocking observation: the |
Fixes #1611
Owner: Dev3
Reviewer: Reviewer
Written-by: Dev-3
Branch:
dev3/fix/history-row-idBase:
655aa714609b34836d4b021072f6a3cbe593f0dd(main, #1609)Tip:
c3861bfca5d6988c480149124cb95ca4a576d389Tree:
00a0c75b092064d4fec87c3f6b13f348d8e040e7#1604 taught the Runs tab to mark an older attempt
superseded by <id>, but two of the appends tolane.historypushed a row with noid, andmarkSupersededskips exactly that row (if (!row.id || !newest.id) continue). So when a graph edit stopped a run — the■ 已停止path — and the same task was handed to the workflow again, the stopped attempt sat there like a sibling of the retry instead of the attempt it replaced.What changed (two appends, nothing else)
plugins/workflow/src/service.js:1391(saveGraph): the ended run is archived withhistoryRow(run)instead of a bare{ task, status, at }.runthere is the real runendRunsettled, so the row carries its id.plugins/workflow/src/service.js:1629(purgeDemos):historyRow(run), and no id fallback. In that loopidis the demo map key — the lane/workflow key, exactly whatstate.lanes[id]two lines above indexes — not a run id. Using it would write a fake run identity:retry of <workflow id>in the Runs tab, and a wrong key inrunHistoryRows' dedup. A legacy record that stores no run id now gets a row with no id, whichmarkSupersededskips — the behaviour it has today, guessed nothing.historyRowis defined insidecreateWorkflowServiceandpurgeDemosis module-level, so the builder is passed in (purgeDemos(state, historyRow)); without that,oxlint'sno-undeffails on the line (it did).plugins/workflow/test/history-row-id.test.js— three tests, the first two red onmain:saveGraphend path (no hand-written id) leaves a history row, the same task is assigned again, and the client must mark the stopped attemptsupersededBy=<retry id>withretryOfon the retry. It also asserts the archived row's id equals the ended run's id.wf-xwithrun.id: 'run-demo'gets a row with that id and notwf-x; a record with norun.idgets a row with no id and not its lane key.supersededBy), the attempt with an id beside it is still marked, and the retry names that one.changelog.d/1611-workflow-history-row-id.fixed.md.Acceptance → test
saveGrapharchives at:1391carries the ended run's real id, and the stopped attempt is markedsupersededBythe retrythe attempt a graph edit stopped is marked superseded by the retry (#1611)purgeDemoswrites at:1629carries the stored run id and never the lane key (lanewf-x+run.id: 'run-demo'→run-demo; norun.id→ no id)the demo-store migration never borrows the workflow id for a history row (#1611)markSuperseded, while the retry names the attempt that does carry an ida legacy row with no id stays unmarked and never names a retry (#1611)Not touched:
nodeStatus, any rendered text, the other four appends. Observed and left alone as out of this issue's scope:migrateRuns(:1655) writes the same id-less row when a pre-lane store is upgraded.Red first, at
655aa714(source untouched, only the test file added)cd plugins/workflow && node --import ./test/resolve.mjs --test test/history-row-id.test.js— 3 tests / 1 pass / 2 fail:not ok 1 - the attempt a graph edit stopped is marked superseded by the retry (#1611)—the stopped attempt names the retry that replaced it: actualundefined, expected the retry's run id.not ok 2 - the demo-store migration never borrows the workflow id for a history row (#1611)—the row carries the stored run id: actualundefined, expectedrun-demo.ok 3 - a legacy row with no id stays unmarked and never names a retry (#1611)— green onmainby design: it locks the behaviour that must not change.Green at this tip
test/history-row-id.test.js: 3 / 3.plugins/workflow: 217 / 217 (mainis 214, the three tests are the only addition); the existing#1507pair intest/client.test.jsstill passes.TZ=UTC ARCH_DOCS_BASE=655aa714609b34836d4b021072f6a3cbe593f0dd npm_config_cache=<writable> npm run verify— exit 0: core 1902, social 528, tree 125, timeline 129, workflow 217,.github549, interop 37, live-e2e 24, every suite# fail 0; oxlint 0 errors (warnings only: core 63, social 36, timeline 9, tree 2, workflow 15,.github1);arch-docs: OK (3 changed file(s));build-client --checksilent;ds-bot-patches check→ok 10 patches apply cleanly in order+ok core/ equals ds-bot/ + patches/ds-bot/;node .github/scripts/changelog.mjs check→ 3 fragments well formed.npm_config_cacheoverride is my sandbox only:~/.npmis read-only there. Counts are from this tip's tree (00a0c75b); the fork commit is the same tree on the same base (655aa714).Not run: the smoke test and the test bench (no
index.js/core/src/host/change), and no browser check — the marking is asserted throughrunHistoryRowswith rows the host produced, not through a rendered page.Docs-not-needed:
docs/subsystems/workflow.mdalready states that "history records a finished or failed live run" and that a failed sibling appears with its time and reason; this change only puts the id the client already reads into the row. No owned path outsideplugins/workflow/changes.Risk: T2 —
plugins/**sets the tier;plugins/workflow/test/**andchangelog.d/**are T1.Checkpoint
{ "pr": 1613, "claim": "Dev3", "issue": 1611, "summary": "Two appends to lane.history pushed a bare row with no id, so the attempt a graph edit stopped was never marked superseded by its retry. saveGraph and the demo-store migration now write through the same row builder; the migration takes no id fallback because its map key is the lane, not a run, and a record with no run id keeps a row with no id. Three tests use the host's real shapes: the saveGraph row gains the run id, the migrated row never borrows the lane key, and an id-less legacy row stays unmarked while the retry names the attempt that does have an id.", "files": [ "plugins/workflow/src/service.js", "plugins/workflow/test/history-row-id.test.js", "changelog.d/1611-workflow-history-row-id.fixed.md" ], "checks": { "ran": "cd plugins/workflow && node --import ./test/resolve.mjs --test test/history-row-id.test.js; cd plugins/workflow && npm test; TZ=UTC ARCH_DOCS_BASE=655aa714609b34836d4b021072f6a3cbe593f0dd npm run verify; node .github/scripts/changelog.mjs check", "result": "green", "at": "c3861bfca5d6988c480149124cb95ca4a576d389" }, "next": "none — ready for review" }